Repository navigation
GH-51368: [C++] Fix silent LF drop when quoted CRLF straddles CSV blo… - #54059
AdityRajGupta wants to merge 2 commits into
Conversation
…SV block boundary
|
|
| // that may follow it (as was done previously) silently corrupts CRLF | ||
| // sequences inside quoted fields (GH-51368). | ||
| pending_cr_ = true; | ||
| buf = SliceBuffer(std::move(buf), offset, buf->size() - offset - 1); |
There was a problem hiding this comment.
This can move buf before evaluating buf->size(), since SliceBuffer takes the buffer by value - QuotedCRLFStraddlingBlockBoundary crashes on my macOS ARM64 Release build. Could we calculate the length before moving buf?
There was a problem hiding this comment.
Thanks for the catch. Yes, the slice length could be evaluated after buf was moved. I now calculate the slice length before moving buf, and checked the function for the same pattern. The quoted CRLF boundary regression test is included, and the full CSV suite passed on Linux.
| offset = 0; | ||
| } | ||
|
|
||
| if (buf->size() > offset && buf->data()[buf->size() - 1] == '\r') { |
There was a problem hiding this comment.
With block_size=4, input "a,b\r1,2\r" has a complete header in the first block. Withholding its CR sends only "a,b" to ProcessHeader, which fails.
After fixing the move issue below, I get this failure in serial, async and streaming readers. The parent implementation reads the row correctly. Could we preserve this case and add regression coverage?
There was a problem hiding this comment.
Withholding the first buffer's trailing '\r' left ProcessHeader without the header terminator. I now hold that first buffer and prepend it to the next one, so your exact block_size=4 example works. I added coverage for serial, async, and streaming readers, including CR and CRLF inputs at block sizes 4 and 64.
…eserve first-row terminator at block boundary
Rationale for this change
With newlines_in_values=true, a \r\n inside a quoted field can be split across two read blocks. When \r is the last byte of one block and \n is the first byte of the next, the reader drops the \n and corrupts the field value. See #51368.
What changes are included in this PR?
CSVBufferIterator now keeps a trailing \r and adds it to the next buffer instead of dropping the following \n. This keeps CRLF together across block boundaries. A pending \r at EOF is also preserved.
Are these changes tested?
Yes.
Are there any user-facing changes?
No.
This PR contains a "Critical Fix": it fixes (b) a bug that caused incorrect data to be produced.
Was AI used for this PR?
Yes AI was used for assistance during the development process mainly in understanding the issue.
Added regression tests for the serial, async, and streaming readers.
These particular test cases were failing before the fix. All 3 now pass with the fix.
PR code and description written by:
Reviewed before submission by: